Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deny requerst if :authority field is invalid only with CONNECT method #612

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

arthurlm
Copy link

@arthurlm arthurlm commented Apr 1, 2022

Looking at RFCs:

  1. RFC 7540: HTTP V2@section 8.1.2.3. Request Pseudo-Header Fields.

The ":authority" pseudo-header field includes the authority
portion of the target URI ([RFC3986], Section 3.2). The authority
MUST NOT include the deprecated "userinfo" subcomponent for "http"
or "https" schemed URIs.

All HTTP/2 requests MUST include exactly one valid value for the
":method", ":scheme", and ":path" pseudo-header fields, unless it is
a CONNECT request (Section 8.3). An HTTP request that omits
mandatory pseudo-header fields is malformed (Section 8.1.2.6).

  1. RFC 3986: Uniform Resource Identifier@section 3.2. Authority

Many URI schemes include a hierarchical element for a naming
authority so that governance of the name space defined by the
remainder of the URI is delegated to that authority (which may, in
turn, delegate it further).

The authority component is preceded by a double slash ("//") and is
terminated by the next slash ("/"), question mark ("?"), or number
sign ("#") character, or by the end of the URI.

Non-validating parsers (those that merely separate a URI reference into
its major components) will often ignore the subcomponent structure of
authority, treating it as an opaque string from the double-slash to
the first terminating delimiter, until such time as the URI is
dereferenced.

  1. RFC 7540: HTTP [email protected]. The CONNECT Method

In HTTP/2, the CONNECT method is used to establish a tunnel over a
single HTTP/2 stream to a remote host for similar purposes. The HTTP
header field mapping works as defined in Section 8.1.2.3 ("Request
Pseudo-Header Fields"), with a few differences. Specifically:

o The ":authority" pseudo-header field contains the host and port to
connect to (equivalent to the authority-form of the request-target
of CONNECT requests (see [RFC7230], Section 5.3)).

A CONNECT request that does not conform to these restrictions is
malformed (Section 8.1.2.6).

So, from my understanding:

  • Following (1):

    • :authority field is not mandatory in HTTP2.
    • It should be a valid URI without scheme and userinfo.
  • Following (2):

    • URI can include sub path component (even if / character is not expected for regular authority).
    • There are HTTP2 clients in the wild that might send "invalid" :authority. It should be treated as opaque string.
  • Following (3):

    • :authority field should be a valid host and port in the case of CONNECT method.

NOTE 1: I have not read the whole RFCs but just read in details the mentioned sections. If anyone have better understanding of this RFCs, please feel free to comment / edit this PR 😉 !

NOTE 2: Please have a look at my first comment about k8s usage with tonic for more details.


This change could fix a lot of already referenced issues / PRs using h2 and gRPC / tonic.

It will also:

  • Improve support for gRPC call using go clients
  • Allow people to develop nice k8s extension with tonic (which is not possible for now without forking h2)
  • Help h2 to better fit to HTTP2 RFC specification 😁

Copy link

@bachp bachp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes it possible to use h2 with k8s.

@bachp
Copy link

bachp commented May 30, 2022

@seanmonstar do you see any issue with this change?

@Forsworns
Copy link

Any update on this one? Just met the same problem when communicate with k8s via tonic :(

@tiagolobocastro
Copy link

Any update on this? Would be good to stop using a fork :)

@mythi
Copy link

mythi commented Feb 8, 2023

Any update on this one? Just met the same problem when communicate with k8s via tonic :(

k8s 1.26 kubelet sets localhost authority for device-plugin and CSI.

@arthurlm
Copy link
Author

arthurlm commented Feb 8, 2023

Even if recent version of k8s set localhost for :authority field:

  • not everyone has access to latest version of k8s (and in big company it can takes years)
  • in cloud as service provider (EKS for example) latest version of k8s is still 1.24
  • h2 still does not respect HTTP2 RFC without this change

I do not really understand why maintainers do not want to review / merge / close this PR.
I guess it is probably dues to some lack of time or PR visibility / interest.

I still hope this will be merged or discussed one day 😄.
So this is why I have not closed this for now...

@mythi
Copy link

mythi commented Feb 8, 2023

@arthurlm FWIW I agree with you. Just wanted to share what I know k8s kubelets can do today.

diconico07 added a commit to diconico07/akri that referenced this pull request Oct 18, 2023
Opcua crate patch update required because of
[opcua#294](locka99/opcua#294)

h2 patch needed because of the bad/missing Authority header, using
upstream PR branch for this
[h2#612](hyperium/h2#612)

Signed-off-by: Nicolas Belouin <[email protected]>
diconico07 added a commit to diconico07/akri that referenced this pull request Oct 18, 2023
Opcua crate patch update required because of
[opcua#294](locka99/opcua#294)

h2 patch needed because of the bad/missing Authority header, using
upstream PR branch for this
[h2#612](hyperium/h2#612)

Signed-off-by: Nicolas Belouin <[email protected]>
diconico07 added a commit to diconico07/akri that referenced this pull request Oct 23, 2023
Opcua crate patch update required because of
[opcua#294](locka99/opcua#294)

h2 patch needed because of the bad/missing Authority header, using
upstream PR branch for this
[h2#612](hyperium/h2#612)

Signed-off-by: Nicolas Belouin <[email protected]>
kriswuollett pushed a commit to appbiotic/h2 that referenced this pull request Nov 3, 2023
diconico07 added a commit to diconico07/akri that referenced this pull request Nov 16, 2023
Opcua crate patch update required because of
[opcua#294](locka99/opcua#294)

h2 patch needed because of the bad/missing Authority header, using
upstream PR branch for this
[h2#612](hyperium/h2#612)

Signed-off-by: Nicolas Belouin <[email protected]>
diconico07 added a commit to diconico07/akri that referenced this pull request Nov 17, 2023
Opcua crate patch update required because of
[opcua#294](locka99/opcua#294)

h2 patch needed because of the bad/missing Authority header, using
upstream PR branch for this
[h2#612](hyperium/h2#612)

Signed-off-by: Nicolas Belouin <[email protected]>
diconico07 added a commit to project-akri/akri that referenced this pull request Jan 23, 2024
* Use upstream version of h2

Go back to upstream h2 version as the go-grpc bug is long solved

Signed-off-by: Nicolas Belouin <[email protected]>

* webhook: Upgrade actix

Upgrade actix, actix-web and actix-rt to latest,
This solves the following audit issues:
 - RUSTSEC-2020-0016
 - RUSTSEC-2020-0056
 - RUSTSEC-2021-0124
 - RUSTSEC-2023-0034

Signed-off-by: Nicolas Belouin <[email protected]>

* Update all dependencies

Opcua crate patch update required because of
[opcua#294](locka99/opcua#294)

h2 patch needed because of the bad/missing Authority header, using
upstream PR branch for this
[h2#612](hyperium/h2#612)

Signed-off-by: Nicolas Belouin <[email protected]>

* Upgrade to 2021 edition

This is needed to be able to upgrade prost dependency

Signed-off-by: Nicolas Belouin <[email protected]>

* Update to rust 1.73.0

Signed-off-by: Nicolas Belouin <[email protected]>

* Fix clippy errors/warning with new rust version and edition

Signed-off-by: Nicolas Belouin <[email protected]>

* Update tonic and prost

Signed-off-by: Nicolas Belouin <[email protected]>

* Change uri used for patched h2

Signed-off-by: Nicolas Belouin <[email protected]>

* Update patch version

Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* Remove patch for opcua as upstream made release

Signed-off-by: Nicolas Belouin <[email protected]>

* Also upgrade mockall

Signed-off-by: Nicolas Belouin <[email protected]>

---------

Signed-off-by: Nicolas Belouin <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants